OLS-3450: Add credentialHotReload flag to skip app-server restart on LLM secret rotation - #1779

@lalan7: This pull request references OLS-3450 which is a valid jira issue.

sequenceDiagram
  participant OLSConfig
  participant OLSConfigReconciler
  participant LLMSecret
  participant AppServerConfig
  participant SecretWatcherFilter
  participant AppServerDeployment

  OLSConfig->>OLSConfigReconciler: provide CredentialHotReload
  OLSConfigReconciler->>AppServerConfig: set credential_hot_reload
  OLSConfigReconciler->>LLMSecret: remove watcher annotation
  LLMSecret-->>SecretWatcherFilter: credential secret event
  alt hot-reload enabled
    SecretWatcherFilter-->>AppServerDeployment: no restart trigger
  else hot-reload disabled
    SecretWatcherFilter->>AppServerDeployment: trigger restart
  end

_{Comment @coderabbitai help to get the list of available commands.}

Hi @blublinsky — this is the operator companion to service PR openshift/lightspeed-service#2955 (RFE-9380).

Adds an opt-in credentialHotReload boolean to the OLSConfig CRD. When enabled, the operator skips rolling restarts for LLM credential secret rotations — the service handles it in-process. Default is false, so no behavior change for existing clusters.

All CI checks pass. The bundle-e2e-4-21 failure is a pre-existing main issue (unknown fields agenticConsole/alertsAdapter in test fixtures from PRs #1758/#1759), not related to this change.

Would appreciate a review when you have a chance.

Suggestion: drive hot-reload from ForEachExternalSecret instead of SecretWatcherFilter

The flag is currently wired in two places: LLMSecretNames + CredentialHotReload are populated before ForEachExternalSecret, and SecretWatcherFilter has a runtime skip branch. That duplicates the provider iteration and adds special-case logic on the event path.

A simpler approach is to handle this inside the ForEachExternalSecret callback, the same way TLS secrets get their AnnotatedSecretMapping today: when credentialHotReload is enabled and source is llm-provider-*, do not annotate that secret (skip annotateSecretIfNeeded). Without the watcher annotation, shouldWatchSecret never admits it, so no restart watcher is created for that secret — no .data events on the hot-reload path, no SecretWatcherFilter changes, no LLMSecretNames map, no extra fields on WatcherConfig, no skip logic at event time.

When the flag is off, behavior stays as today: foreach annotates LLM credential secrets and rotations trigger a rolling restart as they do now.

That keeps “which secrets does the operator react to?” entirely in the existing foreach/mapping configuration model, instead of splitting it between reconcile-time config and filter-time exceptions.

One thing to nail when toggling the flag on for clusters that already have annotated LLM secrets: reconcile may need to remove the watcher annotation when hot-reload is enabled, not only skip adding it for new ones.

I’d hold off on deeper review until this is reworked — the foreach-based approach removes most of the PR’s watcher-layer changes and is the pattern we already use for other secret types.

Follow-up: credentialHotReload must reach the service via OLS config for end-to-end behavior

Today this flag lives only on the CR and in operator WatcherConfig (skip restart). It is not propagated into the app-server olsconfig ConfigMap (buildOLSConfig / AppSrvConfigFile) and the service PR has no corresponding config field either.

That leaves two independent behaviors with no runtime coupling:

For this to work end-to-end, the flag should flow:

OLSConfig CR (spec.ols.credentialHotReload)
  → operator builds olsconfig ConfigMap
  → service reads config and enables hot-reload credential reads when true

Without that, enabling the CR flag on a cluster with an older service image (or before #2955 is deployed) silently disables restarts while credentials stay stale at startup — the CR comment warns about this, but nothing in config enforces it.

Suggest:

1. Add the field to the generated OLS config (utils.OLSConfig / buildOLSConfig).
2. Service side: gate get_credentials() disk re-read on that config flag (default false for backward compatibility).
3. Operator watcher skip (ideally via ForEachExternalSecret / no annotation) should follow the same CR flag.

That gives one user-facing switch that coordinates both “don’t restart” and “do re-read from disk,” instead of two repos assuming the other half is always correct.

Scoping: operator-only PR + bundle follow-up

This PR is correctly scoped to the operator only — service changes stay in openshift/lightspeed-service#2955. Keep it that way.

Once the operator changes here are in good shape, suggest landing this PR without blocking on the OLM bundle, then a follow-up PR that runs make manifests && make bundle (or hack/update_bundle.sh) so bundle/manifests/ols.openshift.io_olsconfigs.yaml picks up credentialHotReload. Right now the field is only in config/crd/bases/; OLM installs use the bundle CRD, so the feature isn’t usable from the shipped bundle until that follow-up.

Order I’d expect:

1. Operator PR (this repo) — CRD + reconcile/watcher wiring + config pass-through to olsconfig once agreed
2. Service PR — gated get_credentials() re-read on config flag
3. Bundle follow-up — regenerate committed bundle/ manifests

That keeps review focused and matches how we usually ship CRD schema vs bundle image updates.

@blublinsky

Addressed all code review findings in the latest push:

Service (lightspeed-service):

Consolidated AWS credential reads — Bedrock.load() now calls get_aws_credentials() once and passes the tuple through to _build_boto3_session() / _build_sigv4_auth() via an optional parameter. This cuts disk reads per load() from 6 to 3 and eliminates a potential mid-request partial rotation race (e.g. new access_key + old secret_key if rotation lands between calls).

Guarded optional role_arn read — get_aws_credentials() now checks os.path.isfile() before attempting to read the role_arn file, avoiding spurious warning logs when it's not configured (common scenario).

Operator (lightspeed-operator):

Collected removeSecretAnnotationIfNeeded errors — failures are now appended to the errs slice, consistent with annotateSecretIfNeeded error handling. Previously these were logged but silently swallowed.
Replaced hardcoded annotation key in e2e test — all 3 instances of "ols.openshift.io/watcher" now use utils.WatcherAnnotationKey.

Added unit tests for removeSecretAnnotationIfNeeded — covers annotation present (removed), annotation absent (no-op), and secret not found (no-op).

All service tests pass (28/28). Operator compiles and passes go vet cleanly (envtest unit tests require kubebuilder binaries available only in CI).

/retest

/retest

we are going to have a discussion reviewing the design of this feature and then decide how to proceed.

/retest

@raptorsun — thanks for the heads up. Happy to join the design discussion or provide any additional context on the approach. Just to summarize where things stand:

I implemented all of @blublinsky's feedback (annotation-based approach via ForEachExternalSecret, ConfigMap propagation to the service, gated get_credentials() on the flag)
All Prow CI checks are green on both PRs (operator #1779 + service #2955)
The feature is opt-in (credentialHotReload: false by default), no behavior change for existing clusters
Please let me know when the discussion is scheduled — I'd like to be there to answer any questions. Thanks!

should-fix: "How It Works" section describes the old watcher-filter approach, not the current annotation-based implementation

docs/credential-hot-reload.md lines 44–52 still describe the original approach (commit 1) where the flag was stored in WatcherConfig and the watcher filter had skip logic. The reworked approach (commit 2) is entirely different — the operator removes the watcher annotation from LLM secrets, so the watcher predicate never admits the event.

Suggested rewrite for "How It Works":

1. During reconciliation, if credentialHotReload is enabled and the secret source is llm-provider-*, the operator removes the watcher annotation (or skips adding it for new secrets).
2. Without the annotation, the watcher predicate (shouldWatchSecret) does not admit the event — no restart happens.
3. When the flag is toggled off, the annotation is re-added on the next reconciliation, restoring restart behavior.
4. Non-LLM secrets (TLS, MCP, Postgres) are annotated and trigger restarts regardless of the flag.

should-fix: Squash commits before merge

The PR has 6 commits including incremental review fixes and a revert (commit 5 is reverted by commit 6). Per repo conventions, please squash to a single logical commit before merge.

Please regenerate the generated manifests using make manifests and make bundle.

/retest

/lgtm
/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: blublinsky

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/retest-required

Remaining retests: 0 against base HEAD e537fb5 and 2 for PR HEAD c2684ce in total

/retest

All PipelineRuns for this commit have already succeeded. Use /retest <pipeline-name> to re-run a specific pipeline or /test to re-run all pipelines.

/retest

All PipelineRuns for this commit have already succeeded. Use /retest <pipeline-name> to re-run a specific pipeline or /test to re-run all pipelines.

/retest

All PipelineRuns for this commit have already succeeded. Use /retest <pipeline-name> to re-run a specific pipeline or /test to re-run all pipelines.

/test "Red Hat Konflux / bundle-e2e-tests / ols-bundle"

/test

/retest

/override "Red Hat Konflux / operator-e2e-tests-direct / lightspeed-operator"
/lgtm

@blublinsky: Overrode contexts on behalf of blublinsky: Red Hat Konflux / operator-e2e-tests-direct / lightspeed-operator

@lalan7: all tests passed!

Full PR test history. Your PR dashboard.